-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix to non-unique LAMMPS molecule IDs #999
Conversation
…emically-identical Molecules in a Topology
for more information, see https://pre-commit.ci
I'll let @mattwthompson give the final review when he's back next week, but in the meantime, there are three things that would improve this PR:
|
Can do! To address point-by-point:
|
+1 on introducing these changes from a branch instead of a fork. Please also base the changes off of the current Feel free to focus these changes on LAMMPS but please do add a comment like "TODO: See if this change is also needed in the GROMACS/Amber exporters" so it's at least noted somewhere. I'd be happy if Patrick or somebody carried the torch to another changeset (though I'm not sure if this problem also applied to GROMACS files).
Your tests can live there or here or anywhere that looks halfway appropriate. Interchange's test suite isn't very well-organized at the moment and I'm more concerned about the tests existing than being in the right place immediately.
Not really. Formatting is handled by automation. I'd like to think our tests follow "Arrange-Act-Assert" either in spirit or in a partially literal sense. What I look for in reviewing is if I can understand what the test is trying to do and that the test actually checks everything that it sets out to check (sometimes this takes several
Yep, precisely. I have a marginal preference for creating topologies on the fly vs. reading files, but if you already have PDB files sitting around those would be fine |
Alrighty, I've moved my changes to a new branch (" For full disclosure, I've never used Pytest before and wasn't clear what the role of the decoration and formatting of the TestLammps class was, so I wrote my test to simply return bool (True if passing, False otherwise). I imagine this is relatively easy to reformat if needed, but I would be interested to know what's involved in that if that's the case. In an environment made from the dev_env.yaml requirements on my machine, the tests runs and returns as expected, namely False with the current Hope this will pass the review, if so I'd be happy to dig into the related GROMACS/AMBER issue and put forth further LAMMPS improvements as I spot them. Thanks! |
Well, ya fooled me. You seem to understand enough to write tests, but here's a good resource on the basics if you want to start from the ground up again: https://education.molssi.org/python_scripting_cms/08-testing/index.html
There isn't much of consequence in the structure of the |
Description
Addresses issue 998, wherein distinct but chemically-identical molecules are forced to have the same molecule ID when writing the atom table section of LAMMPS files.
Checklist